Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[10.x] Validate version and variant in Str::isUuid() #48321

Merged
merged 1 commit into from
Sep 7, 2023
Merged

[10.x] Validate version and variant in Str::isUuid() #48321

merged 1 commit into from
Sep 7, 2023

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Sep 6, 2023

Right now, Str::isUuid() will return true for UUIDs that are not RFC4122-compliant. In addition to the 8-4-4-4-12 format, UUIDs have two special characters:

  • The 13th character (not counting dashes) is the "version" — which can be 1–5
  • The 17th character (not counting dashes) is the "variant" — which can be 8, 9, a, or b

(These special values are actually defined by specific bits in the UUID, but in practice it works out to the above rules.)

Here are two UUIDs that look valid but actually are not:

ff6f8cb0-c57d-11e1-cb21-0800200c9a66
                   ^ "c" is not a valid variant

ff6f8cb0-c57d-61e1-9b21-0800200c9a66
              ^ "6" is not a valid version

This PR updates the regular expression to account for these constraints on validity. It also accounts for the one outlier case, which is the "NIL" UUID 00000000-0000-0000-0000-000000000000. Rather than complicating the regular expression, I've just added an additional check for this exact string, because it's the only of its kind.

@henzeb
Copy link
Contributor

henzeb commented Sep 6, 2023

Never looked into it, but since laravel comes with Ramsey's Uuid package, why not use it's isValid method?

@inxilpro
Copy link
Contributor Author

inxilpro commented Sep 6, 2023

Never looked into it, but since laravel comes with Ramsey's Uuid package, why not use it's isValid method?

I took a peek and Ramsey's package has the same, more generic, expression. The uuidjs package uses this same approach, though.

@taylorotwell
Copy link
Member

Could this be a breaking change for UUIDs that Laravel currently generates like "ordered" UUIDs, etc.?

@inxilpro
Copy link
Contributor Author

inxilpro commented Sep 6, 2023

I checked ordered uuids and they’re fine. I was 99% sure they’d be unaffected, but I also tested a few million random ordered UUIDs just in case. Are there other formats that I should double-check?

@taylorotwell
Copy link
Member

Str::uuid and Str::orderedUuid are the only Laravel UUID methods, so I don't think so.

GrahamCampbell
GrahamCampbell requested changes Sep 6, 2023
View reviewed changes
src/Illuminate/Support/Str.php
@@ -516,7 +516,11 @@ public static function isUuid($value)
return false;
}

return preg_match('/^[\da-f]{8}-[\da-f]{4}-[\da-f]{4}-[\da-f]{4}-[\da-f]{12}$/iD', $value) > 0;
if ($value === '00000000-0000-0000-0000-000000000000') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we allowing this bad null value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The NIL UUID is valid per the RFC.

@taylorotwell taylorotwell merged commit 1591fed into laravel:10.x Sep 7, 2023
coppolafab pushed a commit to coppolafab/laravel-framework that referenced this pull request Sep 8, 2023
@inxilpro @coppolafab
Validate version and variant for UUID (laravel#48321)
50f2028
@nizacegodk nizacegodk mentioned this pull request Sep 13, 2023
Closed
taylorotwell added a commit that referenced this pull request Sep 13, 2023
@taylorotwell
Revert "Validate version and variant for UUID (#48321)"
2980f53 
This reverts commit 1591fed.
@taylorotwell taylorotwell mentioned this pull request Sep 13, 2023
Merged
taylorotwell added a commit that referenced this pull request Sep 13, 2023
@taylorotwell
Revert "Validate version and variant for UUID (#48321)" (#48385)
f5c86ee 
This reverts commit 1591fed.
@frodeknutsen
Copy link
Contributor

This PR introduced a breaking change for ULIDs that are converted to UUID.

@driesvints
Copy link
Member

@frodeknutsen we've reverted this in https://github.com/laravel/framework/releases/tag/v10.23.1

@inxilpro
Copy link
Contributor Author

Gah. I was talking specifically about this PR when I tweeted this…

image

Theoretically this change is better per the spec, but I should have just left well enough along. Sorry about that!

@henzeb
Copy link
Contributor

henzeb commented Sep 15, 2023

I'd say re-add it, but use strict in some way?

@GrahamCampbell
Copy link
Member

Why not just compare against '00000000-0000-0000-0000-000000000000' in your own code if you need to allow that value?

@smares
Copy link

smares commented Sep 17, 2023 •
edited
Loading

Why not just compare against '00000000-0000-0000-0000-000000000000' in your own code if you need to allow that value?

Why? It's a valid UUID as per https://datatracker.ietf.org/doc/html/rfc4122#section-4.1.7

I expect an isUuid method to return true for the valid Nil UUID without having to take it into account myself as a developer.

@edcheyjr edcheyjr mentioned this pull request Sep 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GrahamCampbell GrahamCampbell GrahamCampbell requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@inxilpro @henzeb @taylorotwell @frodeknutsen @driesvints @GrahamCampbell @smares